-
Notifications
You must be signed in to change notification settings - Fork 873
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OpenTelemetryDriver don't require DriverManager for underlying drivers #7089
OpenTelemetryDriver don't require DriverManager for underlying drivers #7089
Conversation
|
...on/jdbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/OpenTelemetryDriver.java
Outdated
Show resolved
Hide resolved
...on/jdbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/OpenTelemetryDriver.java
Outdated
Show resolved
Hide resolved
...library/src/test/groovy/io/opentelemetry/instrumentation/jdbc/OpenTelemetryDriverTest.groovy
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @michalvavrik. It looks ok to me.
@brunobat how are you? For this PR to be approved, do we need any other approver? |
Not mandatory, but it would be nice |
@brunobat ok thanks, then you could do the merge? |
@brianmolinaspring I asked Bruno to review this as I needed his expertise, but I think we will have to wait for the project maintainers (rights + they know best what's desired here). |
@michalvavrik Perfect, this would solve, to place the driverManager manually, that is, we would not have to do anything else but only update the libraries, right? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx @michalvavrik!
Is there any use-case for this outside of building native images? I'm thinking about it from docs and public API surface perspective.
...on/jdbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/OpenTelemetryDriver.java
Outdated
Show resolved
Hide resolved
...on/jdbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/OpenTelemetryDriver.java
Outdated
Show resolved
Hide resolved
...on/jdbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/OpenTelemetryDriver.java
Outdated
Show resolved
Hide resolved
hey @michalvavrik, would this be an alternative option? https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/jdbc/library/README.md#datasource-way just trying to understand the use case and restrictions better. if you happen to be able to join our Thu 9am UTC-8 SIG meeting, that would be a great place to discuss further, since there are a lot of gaps in knowledge on our side about native images |
@trask your digging is perfectly fine, I used Agroal with OpenTelemetryDriver and judging from OpenTelemetryDataSource and AgroalDataSource, it should be possible. I'll try to find out why we want with the driver and get back to you (as for meeting, can't tell ATM). Thank you for the time you spent reviewing this. |
@trask, For native mode, all classes that will be used need to be known at runtime. This will affect all Drivers. This means that we cannot create a new Otel Driver Wrapper object at runtime if tracing is needed. This needs to be known at build time. |
thx @brunobat, can you wrap the underlying driver with the OTel wrapper at build time? |
Yes @trask ... That can be done. |
I've got a feeling we are not moving forward to the mutual understanding about the issue and possible solutions, so I'll join the meeting on Thursday as Trask proposed. |
Hey @trask , now that we have clarified the matter during the meeting, please provide feedback on the PR (required changes, suggestions, ...) once you have a time, thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good with the API shape as-is 👍
...on/jdbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/OpenTelemetryDriver.java
Show resolved
Hide resolved
...on/jdbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/OpenTelemetryDriver.java
Outdated
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
closes: #7028
For reasons explained in the linked issue, it might be handy to register drivers directly against
OpenTelemetryDriver
rather than againstDriverManager
. I decided to also go with static registry as drivers are often instantiated connect pools (like Agroal) and it could be difficult to use instance varibles. This PR adds additionalDriver
collection where drivers can be registered. If driver is registered both withOpenTelemtryDriver
andDriverManager
, drivers registered withOpenTelemetryDriver
are preferred.